Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chargeback: Fix rate adjustment rounding bug #13331

Merged
merged 11 commits into from
Jan 23, 2017

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Jan 3, 2017

What

  • clean-up the rate adjusting code
  • fix rounding bug

Reproducer

Consider the following scenario with disk_allocation_metric:

  • hourly rate is very small number per gigabyte
  • adjustment from bytes to gigabytes is very small number as well
  • the product of the two is extremly small number, hits limits and gets rounded

Found in #13269.

@miq-bot add_label chargeback, bug, euwe/no

@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 2 times, most recently from b5d826a to c5ff669 Compare January 3, 2017 16:23
@miq-bot miq-bot added the euwe/no label Jan 5, 2017
@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 2 times, most recently from c8973a2 to 5f37575 Compare January 6, 2017 08:48
# derived_vm_allocated_disk_storage --> bytes
# derived_vm_used_disk_storage --> bytes
# Scale the rate in the unit defined by user -> to the default unit of the metric
MetricUnits = {
Copy link
Contributor

@lpichler lpichler Jan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MetricUnits -> METRIC_UNITS ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A man can learn many languages. But my brain does not scale to remember all the naming conventions. :-(

Thanks for keeping an eye on me, when rubocop is not.

@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 7 times, most recently from 96d5f9e to 3530a8c Compare January 6, 2017 14:30
@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 4 times, most recently from c952f05 to 1d2c1c9 Compare January 17, 2017 14:44
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a big improvement to the organization of the code and readability 👍

@gtanzillo
Copy link
Member

@isimluk It seems the test failures are related.

@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 4 times, most recently from 0477fe0 to e3c91f5 Compare January 20, 2017 10:25
@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch from e3c91f5 to 29d1e70 Compare January 20, 2017 11:34
@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch 4 times, most recently from ee36e91 to 074c6cb Compare January 20, 2017 13:37
This is teached in 7th or 8th grade, so backend developers shall
comprehend.
The adjustment is function of two units. It is not function of a value.
It is independent. It does not depend on each other. The value does not
affect the adjustment measure.
Like they do in algebra classes.
Hash can be re-used as it refers to what MetricRollup table contains.
Then we can memoize it and save some time calculating it again and
again.
Float has insufficient precision. Already 1024**-3 causes problems.
Certain version of ruby (before 2.3.3) have problems with

   BigDecimal.new(1) <= Float::INFINITY

or

   Float::INFINITY >= BigDecimal.new(1)

Forcing BigDecimal to float in this case will not hurt.
@isimluk isimluk force-pushed the fix-rate-adjustment-rounding-bug branch from a6b0aa4 to bb40e8a Compare January 20, 2017 15:45
@miq-bot
Copy link
Member

miq-bot commented Jan 20, 2017

Checked commits isimluk/manageiq@839660c~...bb40e8a with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
5 files checked, 0 offenses detected
Everything looks good. ⭐

@isimluk
Copy link
Member Author

isimluk commented Jan 21, 2017

Thanks!

Specs are now fixed.

@gtanzillo gtanzillo added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 23, 2017
@gtanzillo gtanzillo merged commit fd19ff4 into ManageIQ:master Jan 23, 2017
@isimluk
Copy link
Member Author

isimluk commented Jan 23, 2017

Thanks!

@isimluk isimluk deleted the fix-rate-adjustment-rounding-bug branch January 23, 2017 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants